-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added parFlatMapN
#4243
Added parFlatMapN
#4243
Conversation
To me it seems that the name should be |
After a discussion with @armanbilge, I decided that |
Right, I initially proposed The whole concept of However, if you imagine |
project/Boilerplate.scala
Outdated
- p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) } | ||
- | ||
- def flatParMap$arity[M[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => M[Z])(implicit p: NonEmptyParallel[M]): M[Z] = | ||
- p.flatMap.flatten(p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly about this PR, but I'm confused by the code here (also for the existing method above it). Why is it calling p.flatMap.map
instead of p.apply.map
? Since the Apply
is the parallel instance.
Am I confused or is this a bug? Maybe we need some tests using e.g. EitherNec[String, A]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agree, good catch... So now I wonder too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, no this is completely fine. It's because it's calling vanilla map
on a single argument (a tuple), which is then unpacked by the case
in the lambda. I was mistakenly reading it as calling a mapN
-like method, in which case this would be wrong.
The map
must be consistent between the flatMap
and the apply
by law, and using the apply
instance would require a conversion to the parallel datatype and back. So it's an optimization to use the one from the flatMap
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually ... this is just flatMap
... right? 😂
- p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) } | |
- | |
- def flatParMap$arity[M[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => M[Z])(implicit p: NonEmptyParallel[M]): M[Z] = | |
- p.flatMap.flatten(p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) }) | |
- p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) } | |
- | |
- def flatParMap$arity[M[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => M[Z])(implicit p: NonEmptyParallel[M]): M[Z] = | |
- p.flatMap.flatMap(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is! I run the tests locally just to be sure that I was not missing something syntax related :)
On the second thought... If In other words, having
then instead of
we can get
But it comes at price of adding 22+22 new methods. |
I see your point, but how is this any different than #4009? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Although it would be good to address #4243 (comment) :)
Thanks for picking this up!
The difference is not that huge though, but:
Therefore there no way the That said, I am not strictly against this addition... Just to double check that we understand all the pros and cons we're going to get. |
Ah yes, that's a good point. I only just realized this myself but the implication didn't register. I was curious why they are defined on the |
I am curious too, but I can imagine that one of the reasons might have been to have a consistent behaviour between the That said, this might be a reason to not want a What do you think @satorg? |
To bikeshed this a bit, I think parFlatMap might be a better name: the par part of it happens, then we flatMap. That or maybe parMapFlatten, which at least reads like what is happening: first we parMap then we flatten. |
To keep up with the bikeshedding: I love |
I would also like to add one more point on Now the proposed name I bet that most users who want to turn their |
This is the part that I really struggle with. As we know My intuition for a "
|
Both arguments are valid ones
I would love thea coherence you're describing here, but in the first place I have to admit that once I read in the changelog about the addition of At first, I began searching for the implementation to check whether it was implemented like From a certain perspective, this episode might demonstrate that having
The problem IMHO arises here, in the 1-arity case. I suppose that the first impression of a lot of people in front of a That said, I would like to gather further opinions, we are just 4 people discussing it here and maybe someone else might have an enlightening idea on the right name to pick. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there! In Mouse, we got a request to add parFlatMapN
, but already there is this PR in cats. Can I get TLDR about this work? Personally, I like parMapN
and countless times did the _.parMapN(...).flatten
things.
UPD: 👍 from me to parFlatMapN
name. I love it a bit more.
👋 hi, person-who-raised-the-issue-against-mouse here, I never even considered googling for flatParMap - it's down to taste I guess, but to me parFlatMapN makes most sense (also, if IDE support becomes good, I can go |
Thanks both for chiming in. It seems everyone likes Can anyone at least respond specifically to my point in #4243 (comment)? It's okay if the answer is "I don't care because I want IDE completion" 😂 |
Alright, you asked for it! 😆
What's more important for user – one will get the parallel (concurrent actually) behavior of execution multiple effects using |
Ha, okay, fair point. I guess it's just a coincidence that all the existing implementation follow this pattern so far 😉 I do think it would be a good convention to have. Personally I also think that: map(...).flatten <-> flatMap(...) screams that: parMapN(...).flatten <-> flatParMapN(...) Because this seemed like a convention too. But maybe it's just an implementation detail all along :)
I thought about this for a while and I agree. I think it's pretty easy to see what I would prefer we establish a convention here based on the existing implementations and merge this PR as-is, but I won't stand in the way. Thanks for the response :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment about simplifying by avoiding Kleisli in a test.
test("ParMapN over f should be consistent with parFlatMapN over f lifted in Kleisli") { | ||
forAll { (as: List[Int], bs: List[Int]) => | ||
val f: (Int, Int) => Int = _ + _ | ||
val mf: (Int, Int) => List[Int] = Function.untupled(Kleisli.fromFunction[List, (Int, Int)](f.tupled).run) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what Kleisli is doing here. Couldn't we just do: val mf: (Int, Int) => List[Int] = { (a, b) => List(a + b) }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not very much, my intention was to show that _.parMapN(f)
is equivalent to _.parFlatMapN(mf)
where mf
is f: A => B
lifted in a Kleisli[M, A, B]
where Parallel[M]
exists. It turned out bad since I had to tuple/untuple unluckily (Kleisli.fromFunction(f).run
is way more readable). For the sake of readability I'll use a cleaner impl of that function, gimme sec :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed this
val f: (Int, Int) => Int = (a, b) => a + b
val mf: (Int, Int) => List[Int] = (a, b) => List(a + b)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made a comment, take or leave it.
Answered in the single discussions, thanks for the review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your efforts in putting this to the finish. Great work!
Hello 👋!
Taking inspiration from #4009 I added
flatParMapN
generation inBoilerplate
. Let me know if it works for you or if something needs to be changed.